Conversation
feat: support use allow* multiple times in env, flag and docker labels
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 42 minutes and 23 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughThe pull request updates socket-proxy to support multiple Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI Flags
participant ENV as Environment Variables
participant Labels as Docker Labels
participant Config as Configuration System
participant Regexes as Compiled Regexes
participant Handler as Request Handler
CLI->>Config: Parse multiple allow* values
ENV->>Config: getAllowFromEnv() aggregates SP_ALLOW_*
Labels->>Config: extractLabelData() extracts socket-proxy.allow.*
Config->>Config: Aggregate values by HTTP method
Config->>Regexes: Compile multiple regexes per method
Handler->>Regexes: matchURL() checks path against all regexes
Regexes-->>Handler: Return true if any regex matches
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Signed-off-by: Wolfgang Ellsässer <git2026@wollomatic.dev>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/config/config_test.go`:
- Around line 72-97: The test is flaky due to relying on map iteration order
when comparing regex slices; update regexMapsEqual to perform an
order-insensitive comparison: for each entry in map a (method key), fetch
corresponding bRegexes from map b, convert both aRegexes and bRegexes to slices
of strings via ar.String(), sort both string slices (e.g., using sort.Strings),
then compare lengths and each element for equality; keep the function name
regexMapsEqual and ensure extractLabelData tests remain unchanged so the
comparison no longer depends on element ordering.
In `@internal/config/config.go`:
- Around line 164-171: The current merge adds environment regexes into each
mr[i].regexStrings before flag parsing, causing env values to persist even when
a CLI -allow<METHOD> was provided; update the merge logic in the
getAllowFromEnv/initialization block so that for each mr entry (mr[i].method)
you only append env param{value:v, from:fromEnv} if there are no CLI-supplied
values for that method, or alternatively mark env values and let the compile
loop prefer params with source CLI over fromEnv and discard env entries when CLI
entries exist; adjust handling around getAllowFromEnv, mr[i].regexStrings, param
and fromEnv so CLI flags take precedence per method.
- Around line 649-655: Extract the method token from allowSpec using strings.Cut
(as done later) and compare that token exactly against rx.method instead of
using strings.HasPrefix; allow either the exact token (allowSpec == method) or a
dot-suffix (strings.HasPrefix(allowSpec, method+".")). Update the ContainsFunc
predicate that currently uses strings.HasPrefix to first do method, _, _ :=
strings.Cut(allowSpec, ".") and then return method == rx.method, before calling
compileRegexp(labelValue, method, "docker container label") so only true HTTP
methods are accepted and labels like "socket-proxy.allow.getextra" no longer
match.
In `@README.md`:
- Around line 96-101: Update the README examples so repeated-allow env vars are
shown as separate, copy-pasteable shell assignments instead of a single quoted
token; specifically split the ambiguous example into distinct lines like
SP_ALLOW_GET="/v1\..{1,2}/(version|containers/.*|events.*)" and
SP_ALLOW_HEAD=".*" and present the repeated-GET example as two separate
assignments (e.g., SP_ALLOW_GET="/version" and SP_ALLOW_GET_2="/_ping") so users
can copy each variable directly; adjust surrounding text to make clear
SP_ALLOW_GET and SP_ALLOW_GET_2 are separate env vars.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0a266871-4158-4bce-b97c-4742328eba09
📒 Files selected for processing (7)
README.mdcmd/socket-proxy/handlehttprequest.gointernal/config/config.gointernal/config/config_test.gointernal/config/env.gointernal/config/env_test.gointernal/config/param.go
| if !regexMapsEqual(got, tt.want) { | ||
| t.Errorf("extractLabelData() = %v, want %v", got, tt.want) | ||
| } | ||
| if !reflect.DeepEqual(got2, tt.want2) { | ||
| t.Errorf("extractLabelData() = %v, want %v", got2, tt.want2) | ||
| } | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| func regexMapsEqual(a, b map[string][]*regexp.Regexp) bool { | ||
| if len(a) != len(b) { | ||
| return false | ||
| } | ||
| for method, aRegexes := range a { | ||
| bRegexes, ok := b[method] | ||
| if !ok || len(aRegexes) != len(bRegexes) { | ||
| return false | ||
| } | ||
| for i, ar := range aRegexes { | ||
| if ar.String() != bRegexes[i].String() { | ||
| return false | ||
| } | ||
| } | ||
| } | ||
| return true |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Go specification range over maps iteration order unspecified
💡 Result:
Yes, the Go language specification states that the iteration order over maps using range is unspecified. Maps are defined as unordered groups of elements. The official Go blog "Go maps in action" explicitly confirms: "When iterating over a map with a range loop, the iteration order is not specified and is not guaranteed to be the same from one iteration to the next." Since Go 1.0, the runtime randomizes the iteration order to prevent reliance on any specific order, which could lead to portability issues or bugs if maps are rehashed.
Citations:
- 1: https://go.dev/blog/maps
- 2: https://go.dev/ref/spec
- 3: https://stackoverflow.com/questions/55925822/why-are-iterations-over-maps-random
🏁 Script executed:
# First, find the extractLabelData function
rg -A 20 "func extractLabelData" internal/config/config.goRepository: wollomatic/socket-proxy
Length of output: 1092
🏁 Script executed:
# Also examine the test to see how it sets up expected values
rg -B 10 "Test_extractLabelData" internal/config/config_test.goRepository: wollomatic/socket-proxy
Length of output: 244
🏁 Script executed:
# Check what container.Summary.Labels is
rg -A 5 "type.*Labels" internal/Repository: wollomatic/socket-proxy
Length of output: 49
🏁 Script executed:
# Get more context on the Test_extractLabelData function and the test cases
rg -A 50 "func Test_extractLabelData" internal/config/config_test.go | head -70Repository: wollomatic/socket-proxy
Length of output: 1386
🏁 Script executed:
# Search for the test table definition to see how tt.want is constructed
rg "want.*map\[string\]" internal/config/config_test.go -B 5 -A 5Repository: wollomatic/socket-proxy
Length of output: 995
🏁 Script executed:
# Check if want values are hardcoded or constructed dynamically
rg "tt\.want\s*=" internal/config/config_test.go -B 3 -A 3Repository: wollomatic/socket-proxy
Length of output: 49
🏁 Script executed:
# Check the full extractLabelData function to see if there's any sorting
rg -A 40 "func extractLabelData" internal/config/config.goRepository: wollomatic/socket-proxy
Length of output: 1186
🏁 Script executed:
# Check if there are any other tests or assertions that might handle ordering
rg "regexMapsEqual\|reflect.DeepEqual" internal/config/config_test.go -B 3 -A 1Repository: wollomatic/socket-proxy
Length of output: 49
Fix test flakiness caused by unspecified map iteration order.
The test input uses a map for Docker labels, which has unspecified iteration order in Go. The extractLabelData function iterates over cntr.Labels and appends regexes to slices in the order encountered. With positional comparison in regexMapsEqual, the test can fail randomly when labels are iterated in a different order than expected—for example, "socket-proxy.allow.get.1" before "socket-proxy.allow.get.0" would produce regexes in a different sequence.
Replace the positional comparison with order-insensitive comparison by sorting regex strings before comparing.
🧪 Proposed order-insensitive comparison
import (
"reflect"
"regexp"
+ "slices"
"testing"
"github.com/wollomatic/socket-proxy/internal/docker/api/types/container"
)
@@
func regexMapsEqual(a, b map[string][]*regexp.Regexp) bool {
if len(a) != len(b) {
return false
}
for method, aRegexes := range a {
bRegexes, ok := b[method]
if !ok || len(aRegexes) != len(bRegexes) {
return false
}
- for i, ar := range aRegexes {
- if ar.String() != bRegexes[i].String() {
- return false
- }
+ aStrings := regexStrings(aRegexes)
+ bStrings := regexStrings(bRegexes)
+ slices.Sort(aStrings)
+ slices.Sort(bStrings)
+ if !slices.Equal(aStrings, bStrings) {
+ return false
}
}
return true
}
+
+func regexStrings(regexes []*regexp.Regexp) []string {
+ values := make([]string, 0, len(regexes))
+ for _, rx := range regexes {
+ values = append(values, rx.String())
+ }
+ return values
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if !regexMapsEqual(got, tt.want) { | |
| t.Errorf("extractLabelData() = %v, want %v", got, tt.want) | |
| } | |
| if !reflect.DeepEqual(got2, tt.want2) { | |
| t.Errorf("extractLabelData() = %v, want %v", got2, tt.want2) | |
| } | |
| }) | |
| } | |
| } | |
| func regexMapsEqual(a, b map[string][]*regexp.Regexp) bool { | |
| if len(a) != len(b) { | |
| return false | |
| } | |
| for method, aRegexes := range a { | |
| bRegexes, ok := b[method] | |
| if !ok || len(aRegexes) != len(bRegexes) { | |
| return false | |
| } | |
| for i, ar := range aRegexes { | |
| if ar.String() != bRegexes[i].String() { | |
| return false | |
| } | |
| } | |
| } | |
| return true | |
| func regexMapsEqual(a, b map[string][]*regexp.Regexp) bool { | |
| if len(a) != len(b) { | |
| return false | |
| } | |
| for method, aRegexes := range a { | |
| bRegexes, ok := b[method] | |
| if !ok || len(aRegexes) != len(bRegexes) { | |
| return false | |
| } | |
| aStrings := regexStrings(aRegexes) | |
| bStrings := regexStrings(bRegexes) | |
| slices.Sort(aStrings) | |
| slices.Sort(bStrings) | |
| if !slices.Equal(aStrings, bStrings) { | |
| return false | |
| } | |
| } | |
| return true | |
| } | |
| func regexStrings(regexes []*regexp.Regexp) []string { | |
| values := make([]string, 0, len(regexes)) | |
| for _, rx := range regexes { | |
| values = append(values, rx.String()) | |
| } | |
| return values | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/config/config_test.go` around lines 72 - 97, The test is flaky due
to relying on map iteration order when comparing regex slices; update
regexMapsEqual to perform an order-insensitive comparison: for each entry in map
a (method key), fetch corresponding bRegexes from map b, convert both aRegexes
and bRegexes to slices of strings via ar.String(), sort both string slices
(e.g., using sort.Strings), then compare lengths and each element for equality;
keep the function name regexMapsEqual and ensure extractLabelData tests remain
unchanged so the comparison no longer depends on element ordering.
| // multiple values per method | ||
| // like SP_ALLOW_GET_0, SP_ALLOW_GET_1, ... | ||
| allowFromEnv := getAllowFromEnv(os.Environ()) | ||
| for i := range mr { | ||
| if val, ok := os.LookupEnv("SP_ALLOW_" + mr[i].method); ok && val != "" { | ||
| mr[i].regexStringFromEnv = val | ||
| if val, ok := allowFromEnv[mr[i].method]; ok && len(val) > 0 { | ||
| for _, v := range val { | ||
| mr[i].regexStrings = append(mr[i].regexStrings, param{value: v, from: fromEnv}) | ||
| } |
There was a problem hiding this comment.
Preserve CLI-over-env precedence per method.
Env regexes are appended before flag parsing, then the compile loop includes both env and CLI values. That contradicts the documented behaviour and can keep a broader SP_ALLOW_* active even when a restrictive -allow<METHOD> flag is provided.
🔒 Proposed precedence fix
// compile regexes for default allowed requests
cfg.AllowLists.Default.AllowedRequests = make(map[string][]*regexp.Regexp)
for _, rx := range mr {
+ hasParamValue := slices.ContainsFunc(rx.regexStrings, func(regexString param) bool {
+ return regexString.from == fromParam
+ })
for _, regexString := range rx.regexStrings {
+ if hasParamValue && regexString.from == fromEnv {
+ continue
+ }
if regexString.value != "" {
location := ""
switch regexString.from {
case fromEnv:
location = "env variable"Also applies to: 253-266
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/config/config.go` around lines 164 - 171, The current merge adds
environment regexes into each mr[i].regexStrings before flag parsing, causing
env values to persist even when a CLI -allow<METHOD> was provided; update the
merge logic in the getAllowFromEnv/initialization block so that for each mr
entry (mr[i].method) you only append env param{value:v, from:fromEnv} if there
are no CLI-supplied values for that method, or alternatively mark env values and
let the compile loop prefer params with source CLI over fromEnv and discard env
entries when CLI entries exist; adjust handling around getAllowFromEnv,
mr[i].regexStrings, param and fromEnv so CLI flags take precedence per method.
| if slices.ContainsFunc(mr, func(rx methodRegex) bool { | ||
| // allowSpec starts with the method name like socket-proxy.allow.get.1 | ||
| return strings.HasPrefix(allowSpec, rx.method) | ||
| }) { | ||
| // extract the method name from allowSpec | ||
| method, _, _ := strings.Cut(allowSpec, ".") | ||
| r, err := compileRegexp(labelValue, method, "docker container label") |
There was a problem hiding this comment.
Require an exact method label or method-plus-dot suffix.
HasPrefix also accepts labels such as socket-proxy.allow.getextra, producing a GETEXTRA allow entry. Parse the method first, then compare it exactly against supported HTTP methods.
🛡️ Proposed stricter label parsing
- if slices.ContainsFunc(mr, func(rx methodRegex) bool {
- // allowSpec starts with the method name like socket-proxy.allow.get.1
- return strings.HasPrefix(allowSpec, rx.method)
- }) {
- // extract the method name from allowSpec
- method, _, _ := strings.Cut(allowSpec, ".")
+ method, _, _ := strings.Cut(allowSpec, ".")
+ if slices.ContainsFunc(mr, func(rx methodRegex) bool {
+ return method == rx.method
+ }) {
r, err := compileRegexp(labelValue, method, "docker container label")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/config/config.go` around lines 649 - 655, Extract the method token
from allowSpec using strings.Cut (as done later) and compare that token exactly
against rx.method instead of using strings.HasPrefix; allow either the exact
token (allowSpec == method) or a dot-suffix (strings.HasPrefix(allowSpec,
method+".")). Update the ContainsFunc predicate that currently uses
strings.HasPrefix to first do method, _, _ := strings.Cut(allowSpec, ".") and
then return method == rx.method, before calling compileRegexp(labelValue,
method, "docker container label") so only true HTTP methods are accepted and
labels like "socket-proxy.allow.getextra" no longer match.
2 participants
Summary by CodeRabbit
New Features
-allow*configuration entries per HTTP method via command-line flags, environment variables, and Docker labels for more granular access control.Documentation